Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

eth, les: drop support for eth/64 #22636

Merged
merged 1 commit into from
Apr 9, 2021
Merged

eth, les: drop support for eth/64 #22636

merged 1 commit into from
Apr 9, 2021

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented Apr 8, 2021 •
edited
Loading

This PR drops support for eth/64. Unfortunately, there's not much more we can do now to clean up our codebase until we also start dropping eth/65. That said, after Berlin it's probably safe to still serve but not sync from below eth/66, which would simplify certain things.

The PR also enabled eth/66 tests at various locations that didn't have them run and fixes them to complete successfully.

@karalabe karalabe added this to the 1.10.3 milestone Apr 8, 2021
@karalabe karalabe requested a review from holiman April 8, 2021 15:08
holiman
holiman reviewed Apr 8, 2021
View reviewed changes
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, there's also some logic within the tx broadcast loop, to check if a peer is 65+ or less, to fall back to broadcast instead of announce. We can remove that too

eth/handler_eth_test.go Outdated Show resolved Hide resolved
@holiman
Copy link
Contributor

holiman commented Apr 8, 2021

Ah, here: https://github.com/holiman/go-ethereum/blob/master/eth/handler.go#L889

@karalabe
Copy link
Member Author

karalabe commented Apr 9, 2021

@holiman Dropped the old broadcast path, PTAL

@holiman
Copy link
Contributor

holiman commented Apr 9, 2021

Got ACK from Besu, Turbo-geth and Nethermind that they speak 65. OE has seen my question, but not given a clear answer yet.

holiman
holiman approved these changes Apr 9, 2021
View reviewed changes
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@karalabe karalabe merged commit 9c653ff into ethereum:master Apr 9, 2021
@holiman
Copy link
Contributor

holiman commented Apr 9, 2021

For context
Screenshot_2021-04-09_15-29-44

@quorumbot quorumbot mentioned this pull request Jul 14, 2022
Closed
9 tasks
@quorumbot quorumbot mentioned this pull request Aug 5, 2022
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holiman holiman holiman approved these changes

@rjl493456442 rjl493456442 Awaiting requested review from rjl493456442 rjl493456442 is a code owner

@zsfelfoldi zsfelfoldi Awaiting requested review from zsfelfoldi zsfelfoldi is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.10.3
Development

Successfully merging this pull request may close these issues.
2 participants
@karalabe @holiman